These are now covered by static type checks.

Make sure these are covered in schema tests?

👍 added tests covering the same behavior in rule_schemas.test.ts

nit: I would try to keep to utilize as many strong types as we can as it's better to fail during static analysis and/or in the IDE than during test runtime. This also gives hints to any developer coming into the file when they're adding a new test by copying/pasting a previous test and modifying they will have the type for it.

For example, with a Partial<CreateRulesSchema> if you misspell something when writing a test like so below you will give faster positive feedback from your IDE rather than waiting for the jest test to run. Also when people unfamiliar with our code base refactor they will get typescript compiler errors first before jest errors and those are typically easier to figure out instead of jest errors.

nit: Same advice with having strong types applies to the payload/expected which is why we break them out and add types as it's easier to find errors, type mismatches, and get positive feedback directly from the IDE rather than rely on Jest.

Typically even in other areas of the codebase when I do refactors if I am working in a Jest test that is using an expect payload without a strong type but I can add one, I usually do add it during the refactor. Later it makes it that much easier to just run type checks, fix the problems all at once and then run the larger jest tests.

That and fixing type issues in my IDE is positive feedback when types/keys change on shapes of JS and I can fix all of the type issues at once rather than running the test, inspecting the output, and then fixing things that TypeScript could have let me know about in the IDE.

👍 on keeping the type here. With a spread and an additional value, we have to explicitly add the type to ensure stricter checks from TS. Awesome!

I think this is ok to do to weaken the types for a test if the Omit feels unergonomic and unwieldily to use to keep the type strict. You should get the majority of type safety from the function getCreateRulesSchemaMock still.

If you hover between the two types though of this payload and the getCreateRulesSchemaMock() you should notice the interesting thing which is TypeScript during a spread is changing the types slightly.

In regular (outside of tests) we have had some odd behaviors from spreads where TypeScript can change the types slightly or have an ambiguity situation where once we added the type to where we initiated the spread the errors/issues went away.

You might know all of this, just footnotes in case.

++ I added back most of the stronger types where the payload is intended to validate correctly or some fields are simply missing. I figure for cases where validation should fail because a field is the wrong type there's not as much benefit so I left them off for convenience.

Optional, if you want to cut down on some the dotted noise you can always do ...just... a bit of destructoring like so:

async (context, request, response) => {
  const { body } = request;

What's interesting is we have been following guidelines from Kibana here:
https://github.com/elastic/kibana/blob/master/STYLEGUIDE.md#use-object-destructuring

But I think as the rules grew in size, no one wants to destructor a large amount of data. I think this is all very readable as is, fwiw 👍

Here I ended up pulling the conversion out into a separate function so it was easy to just pass request.body into that function, best of both worlds.

I think the technical term would be removeNil as this would be removed null and undefined values. The original was removedNullValues but that wasn't accurate as well.

At least nil is the terminology that lodash uses:
https://lodash.com/docs/4.17.15#isNil

No changes asked, just pointing it out.

Took out one of these statements and the exhaustive switch statement looks good and like it is working 👍

Feel free if you want to directly use the booleans:

  if (existingRule.enabled && !enabled) {
    await alertsClient.disable({ id: existingRule.id });
  } else if (!existingRule.enabled && enabled) {
    await alertsClient.enable({ id: existingRule.id });

Instead of the explicit === true or === false, those are probably just older bad habits from too much time with JS types that could be undefined or null, but I think these booleans are ok to use normally if you think that reads better.

No changes requested, just pointing out parts of code myself or others wrote that might read odd.

If it helps, the alerting framework team helped us with this part as we have to put the enabling/disabling of rules after we do any updates first because once we enable/disable a rule it will invalidate API keys and can cause a "false run" to happen where the first run has a bad API key.

So right in this section when we call this line:

await alertsClient.enable({ id: existingRule.id });

That is going to start the rule and invalidate any previous API keys it might have had. So just a FYI for you if you haven't figured that part out yet looking through here.

Optionally you can bump this up to say 30 like so:

/* eslint complexity: ["error", 30]*/

If you don't want it to go higher than 30.

You could break up the params section here to another function and return the params separate if you wanted to reduce the complexity:

{
      author: ruleUpdate.author ?? [],
      buildingBlockType: ruleUpdate.building_block_type,
      description: ruleUpdate.description,
      ruleId: existingRule.params.ruleId,
      falsePositives: ruleUpdate.false_positives ?? [],
      from: ruleUpdate.from ?? 'now-6m',
      // Unlike the create route, immutable comes from the existing rule here
      immutable: existingRule.params.immutable,
      license: ruleUpdate.license,
      outputIndex: ruleUpdate.output_index ?? defaultOutputIndex,
      timelineId: ruleUpdate.timeline_id,
      timelineTitle: ruleUpdate.timeline_title,
      meta: ruleUpdate.meta,
      maxSignals: ruleUpdate.max_signals ?? DEFAULT_MAX_SIGNALS,
      riskScore: ruleUpdate.risk_score,
      riskScoreMapping: ruleUpdate.risk_score_mapping ?? [],
      ruleNameOverride: ruleUpdate.rule_name_override,
      severity: ruleUpdate.severity,
      severityMapping: ruleUpdate.severity_mapping ?? [],
      threat: ruleUpdate.threat ?? [],
      timestampOverride: ruleUpdate.timestamp_override,
      to: ruleUpdate.to ?? 'now',
      references: ruleUpdate.references ?? [],
      note: ruleUpdate.note,
      // Always use the version from the request if specified. If it isn't specified, leave immutable rules alone and
      // increment the version of mutable rules by 1.
      version:
        ruleUpdate.version ?? existingRule.params.immutable
          ? existingRule.params.version
          : existingRule.params.version + 1,
      exceptionsList: ruleUpdate.exceptions_list ?? [],
      ...typeSpecificParams,
    }

Overall it reads fine to me, so I'm ok with leaving it disabled for the file since there's only one function within the file.